feat: add copyable message citation links - #128
Conversation
|
@clawsweeper re-review Exact head |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Codex review: needs changes before merge. Reviewed August 2, 2026, 3:13 AM ET / 07:13 UTC. ClawSweeper reviewWhat this changesThis PR gives channel-root messages stable public citation URLs, backfills eligible existing SQLite and PostgreSQL messages, and adds desktop and touch copy-link actions with an accessible clipboard-denial fallback. Merge readinessKeep this PR open: the previous P1 blocker remains on the unchanged reviewed head. Expanding the migration to every historical channel root performs an unbounded read-then-one-update-per-row pass during startup; PostgreSQL retains its migration advisory lock for that entire interval, so a large existing deployment can be unavailable for an unbounded upgrade period. Priority: P1 Review scores
Verification
How this fits togetherClickClack stores public route IDs separately from internal message IDs and resolves them through existing authorization checks. This PR assigns a stable route ID to each channel-root message, then the web client builds a canonical URL that opens and highlights the root or its thread. flowchart LR
A[Channel-root message] --> B[Store assigns public route ID]
C[Existing SQLite or PostgreSQL data] --> D[Startup citation backfill]
B --> E[Authorized route resolution]
D --> E
E --> F[Web client opens message or thread]
F --> G[Copy-link action and fallback dialog]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Copy recommended automerge instructionTechnical reviewBest possible solution: Backfill legacy channel roots in bounded, durable chunks that can resume without extending startup indefinitely, and add scale coverage for both stores before completing the citation rollout. Do we have a high-confidence way to reproduce the issue? Yes, source-reproducible: seed a large legacy store with route-less channel roots and run migration; the changed predicate selects all roots, then the helper retains all IDs and updates them serially. I did not execute that destructive-scale setup in this read-only review. Is this the best way to solve the issue? No: stable citations are a reasonable feature path, but the proposed eager all-history startup backfill is not a safe upgrade implementation until it is bounded and resumable. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found, but no applicable review policy affected this item. Codex review notes: model internal, reasoning high; reviewed against b8293987baa5. LabelsLabel justifications:
EvidenceAcceptance criteria:
What I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (30 earlier review cycles; latest 8 shown)
|
|
@clawsweeper re-review Exact head: The persistent-route finding was traced to a source-path misread rather than a
This head adds equivalent real-PostgreSQL regression coverage for eager channel The durable PR body now embeds commit-pinned raw screenshots directly and links |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@codex review |
31e7c72 to
a990518
Compare
|
To use Codex here, create a Codex account and connect to github. |
|
@clawsweeper re-review Final head is |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Closing after maintainer review because the startup migration is not safe at production scale. It holds the PostgreSQL advisory migration lock while loading every legacy root into memory and then updating rows individually, so a large installation can block startup and deployment for an unbounded interval. The durable design should assign route IDs lazily or use a bounded, resumable migration with explicit progress. That requires a fresh migration design rather than incremental edits to this branch. The product decision remains tracked in #127. Thanks for the thoughtful implementation work. |
Closes #127
What Problem This Solves
Users and agents can find messages inside ClickClack, but cannot copy a stable, permission-checked link back to a channel message. Decisions and evidence therefore have to be referenced with copied text or vague navigation instructions.
Why This Change Was Made
Channel root messages now receive immutable public
M...route IDs at creation or migration, while replies and new DM citation actions remain out of scope. Desktop and touch action surfaces copy one canonical absolute URL using the configured public frontend origin. Before a first reply the route highlights the root in its channel; afterward the same URL opens the thread pane.Clipboard denial exposes a focused, selected, read-only URL rather than losing the citation. Existing route authorization remains authoritative, so possession of a link grants no access.
Channel and DM roots retain separate persistence paths:
CreateMessageis channel-only and assigns the eager citation identity, whileCreateDirectMessagepreserves the existing lazy DM-thread identity behavior.The upgrade backfill respects the stores’ actual schema histories: SQLite keys off
0011_public_route_ids.sql, while PostgreSQL keys off its initial0001_schema.sql. This makes the citation backfill run on existing PostgreSQL installations instead of silently skipping them.User Impact
People and integrations can cite a durable ClickClack discussion with a copyable link that remains stable as the message becomes a thread. Links use public route IDs only and preserve current workspace, channel, guest, and message permissions. Existing SQLite and PostgreSQL stores receive the same channel-root citation behavior during migration.
Evidence
Exact reviewed head:
98044b55ba89a6fe2f49a928eb3ec8a7ea8ea2f6, based onmainat4843afe2e8eb6bcfeba947496feae3db72e6f659.Built-server browser proof
The production web bundle was built into the Go server and exercised against a fresh real SQLite store:
The exact-head scenario proves desktop and touch copy actions, configured-origin handling, direct-route highlight before replies, the same route opening the thread after its first reply, replies remaining without independent route IDs, and the accessible selected-URL fallback after clipboard denial.
All refreshed screenshots were visually inspected for rendered state, readable controls, stable URL, and layout integrity.
Existing-store upgrade proof
Focused real-store tests passed:
They prove that legacy workspace, channel, direct-conversation, and channel-root rows receive the expected routes; direct-message roots and replies remain uncited; and the one-time completion marker is recorded.
Broader validation
pnpm generate:sqlc— passed with no generated drift.umask 0027; pnpm check— passed.git diff --check origin/main...HEAD— passed.AI-assisted development: yes (Codex); all generated changes were reviewed and validated against the exact head above.